prediction_market_agent_tooling/markets/agent_market.py (1)

299-300: LGTM: New method enhances AgentMarket interface.

The addition of the get_most_recent_trade_datetime method is a valuable enhancement to the AgentMarket class. It provides a standardized way for subclasses to implement retrieval of the most recent trade datetime for a specific user.

Consider adding a brief docstring to explain the method's purpose and return value. For example:

def get_most_recent_trade_datetime(self, user_id: str) -> DatetimeUTC | None:
    """
    Get the datetime of the most recent trade for the specified user.

    Args:
        user_id (str): The ID of the user.

    Returns:
        DatetimeUTC | None: The datetime of the most recent trade, or None if no trades found.
    """
    raise NotImplementedError("Subclasses must implement this method")

tests_integration_with_local_chain/markets/omen/test_omen.py (1)

545-583: LGTM: Comprehensive test for get_most_recent_trade_datetime.

The new test function effectively verifies the behavior of get_most_recent_trade_datetime for both buying and selling trades. It uses appropriate assertions to check that the returned datetime falls within the expected range.

Suggestion for improvement:
Consider adding a small delay between the buy and sell operations to ensure distinct timestamps. This can prevent potential issues on systems with high-resolution timers.

You could add a small delay between buy and sell operations like this:

import time

# After the buy operation and its assertions
time.sleep(0.1)  # 100 ms delay

dt_before_sell_trade = utcnow()
# ... (sell operation code)

This ensures that the buy and sell operations have distinct timestamps, even on systems with high-resolution timers.

prediction_market_agent_tooling/markets/omen/omen.py (1)

661-672: New method implementation looks good, but consider optimizing the trade retrieval.

The new get_most_recent_trade_datetime method is well-implemented and follows the class's coding style. It correctly retrieves trades for a specific user and market, then returns the most recent trade's datetime. However, there are a couple of points to consider:

1. The TODO comment on line 669 suggests that the trades might already be sorted. If this is the case, we can optimize the code by removing the sorting step.

2. If we only need the most recent trade, we could potentially optimize the query to fetch just one trade instead of all trades for the user and market.

Consider the following optimizations:

1. Verify if the trades are already sorted and remove the sorting step if unnecessary.
2. If possible, modify the OmenSubgraphHandler().get_trades() method to accept a limit parameter, allowing us to fetch only the most recent trade:

def get_most_recent_trade_datetime(self, user_id: str) -> DatetimeUTC | None:
    trades = OmenSubgraphHandler().get_trades(
        better_address=Web3.to_checksum_address(user_id),
        market_id=Web3.to_checksum_address(self.id),
        limit=1,
        sort_by='creation_datetime',
        sort_direction='desc'
    )
    return trades[0].creation_datetime if trades else None

This would reduce both the amount of data transferred and the processing required in this method.

Configuration used: CodeRabbit UI
Review profile: CHILL

prediction_market_agent_tooling/markets/omen/omen.py (1)

662-673: Implement sorting optimization and remove TODO comment.

The implementation of get_most_recent_trade_datetime looks good overall. However, there are two points to address:

1. The TODO comment on line 670 should be resolved. If the trades are already sorted by the API, you can remove the sorting step entirely.

2. If the trades are not pre-sorted, you can optimize the method by using the limit parameter in the get_trades call instead of fetching all trades and then sorting them.

Here's a suggested optimization:

def get_most_recent_trade_datetime(self, user_id: str) -> DatetimeUTC | None:
    trades = OmenSubgraphHandler().get_trades(
        better_address=Web3.to_checksum_address(user_id),
        market_id=Web3.to_checksum_address(self.id),
        limit=1,
        sort_by="creation_datetime",
        sort_direction="desc"
    )
    return trades[0].creation_datetime if trades else None

This assumes that OmenSubgraphHandler().get_trades() supports limit, sort_by, and sort_direction parameters. If it doesn't, consider adding these features to improve efficiency.

Configuration used: CodeRabbit UI
Review profile: CHILL

tests/markets/omen/test_omen.py (1)

296-323: Improve test robustness and efficiency.

The test function test_get_most_recent_trade_datetime is a good addition to verify the behavior of get_most_recent_trade_datetime. However, there are a few areas that could be improved:

1. Consider parameterizing the test with different market IDs and user addresses to make it more robust against data changes.
2. The use of sys.maxsize as the limit for get_trades might be excessive. Consider using a more reasonable upper limit or paginating the results.
3. The assertions for multiple trades and unique timestamps might fail if the market conditions change. Consider making these checks conditional or use a different approach to ensure the test's validity.
4. To improve efficiency, you could retrieve a smaller number of trades sorted by creation time in descending order, instead of fetching all trades.

Here's a suggested refactor to address these points:

import pytest
from web3 import Web3

@pytest.mark.parametrize("market_id, user_id", [
    ("0x1e0e1d092bffebfb4fa90eeba7bfeddcebc9751c", "0x2DD9f5678484C1F59F97eD334725858b938B4102"),
    # Add more test cases here
])
def test_get_most_recent_trade_datetime(market_id: str, user_id: str) -> None:
    """
    Tests that `get_most_recent_trade_datetime` returns the correct datetime
    from all possible trade datetimes.
    """
    market_id = Web3.to_checksum_address(market_id)
    user_id = Web3.to_checksum_address(user_id)
    
    sgh = OmenSubgraphHandler()
    trades = sgh.get_trades(
        limit=10,  # Adjust this value as needed
        better_address=user_id,
        market_id=market_id,
        sort_by="creation_time",
        sort_direction="desc"
    )
    
    if not trades:
        pytest.skip(f"No trades found for market {market_id} and user {user_id}")
    
    market = OmenAgentMarket.get_binary_market(id=market_id)
    most_recent_trade_datetime = check_not_none(
        market.get_most_recent_trade_datetime(user_id=user_id)
    )
    
    assert trades[0].creation_datetime == most_recent_trade_datetime, \
        "The most recent trade datetime doesn't match the expected value"

This refactored version addresses the identified issues and makes the test more robust and efficient.

tests_integration_with_local_chain/markets/omen/test_omen.py (1)

320-320: LGTM! Improved test robustness and consistency.

The changes enhance the test by:

1. Consistently using api_keys in method calls, aligning with the updated API structure.
2. Checking for sufficient funds before placing a bet, which is a good practice.

These improvements make the test more robust and consistent with the rest of the codebase.

Consider adding an assertion to verify that the bet amount is less than or equal to the available balance:

assert bet_amount.amount <= balances.xdai + balances.wxdai, "Insufficient funds for the bet"

This would make the fund check more explicit and fail the test if there are insufficient funds.

Also applies to: 326-334, 344-344

prediction_market_agent_tooling/markets/omen/omen_subgraph_handler.py (4)

Line range hint 549-559: LGTM! Consider adding parameter descriptions.

The new parameters limit, sort_by_field, and sort_direction enhance the flexibility of the get_trades method. This is a good improvement that allows for more controlled querying of trades.

Consider adding parameter descriptions in the method's docstring to explain the purpose and expected values for these new parameters, especially sort_by_field and sort_direction.

---

585-595: LGTM! Consider extracting the query parameters logic.

The implementation correctly incorporates the new parameters into the fpmmTrades query. The use of a dictionary for optional parameters is a clean approach.

To improve readability, consider extracting the logic for building the optional_params dictionary into a separate method. This would make the get_trades method more concise and easier to understand. For example:

def _build_query_params(self, sort_by_field: FieldPath | None, sort_direction: str | None) -> dict:
    params = {}
    if sort_by_field is not None:
        params["orderBy"] = sort_by_field
    if sort_direction is not None:
        params["orderDirection"] = sort_direction
    return params

# In get_trades method
optional_params = self._build_query_params(sort_by_field, sort_direction)

---

Line range hint 597-617: Consider updating get_bets to use new parameters.

The get_bets method currently doesn't utilize the new limit, sort_by_field, and sort_direction parameters added to get_trades. While this might be intentional, it's worth considering whether get_bets should also have the ability to limit and sort the results.

Consider updating the get_bets method signature and implementation to include and pass along the new parameters:

def get_bets(
    self,
    better_address: ChecksumAddress | None = None,
    start_time: DatetimeUTC | None = None,
    end_time: t.Optional[DatetimeUTC] = None,
    market_id: t.Optional[ChecksumAddress] = None,
    filter_by_answer_finalized_not_null: bool = False,
    market_opening_after: DatetimeUTC | None = None,
    collateral_amount_more_than: Wei | None = None,
    limit: int | None = None,
    sort_by_field: FieldPath | None = None,
    sort_direction: str | None = None,
) -> list[OmenBet]:
    return self.get_trades(
        better_address=better_address,
        start_time=start_time,
        end_time=end_time,
        market_id=market_id,
        filter_by_answer_finalized_not_null=filter_by_answer_finalized_not_null,
        type_="Buy",  # We consider `bet` to be only the `Buy` trade types.
        market_opening_after=market_opening_after,
        collateral_amount_more_than=collateral_amount_more_than,
        limit=limit,
        sort_by_field=sort_by_field,
        sort_direction=sort_direction,
    )

This change would provide consistent functionality between get_trades and get_bets, allowing users of get_bets to also benefit from the new sorting and limiting capabilities.

---

Line range hint 549-595: Consider applying similar changes to other query methods.

The changes to get_trades are well-implemented and consistent with the existing code style. To maintain consistency across the entire OmenSubgraphHandler class, consider applying similar changes to other query methods that might benefit from limiting and sorting capabilities.

Review other query methods in the class, such as get_omen_binary_markets, get_questions, get_answers, and get_responses. If appropriate, update these methods to include limit, sort_by_field, and sort_direction parameters, and implement the sorting logic similarly to get_trades. This would provide a consistent API across all query methods in the class.

For example:

def get_questions(
    self,
    limit: int | None = None,
    sort_by_field: FieldPath | None = None,
    sort_direction: str | None = None,
    # ... existing parameters ...
) -> list[RealityQuestion]:
    # ... existing implementation ...
    optional_params = self._build_query_params(sort_by_field, sort_direction)
    questions = self.realityeth_subgraph.Query.questions(
        first=(limit if limit else sys.maxsize),
        where=where_stms,
        **optional_params,
    )
    # ... rest of the implementation ...

This would create a more uniform and predictable API across the entire OmenSubgraphHandler class.

Configuration used: CodeRabbit UI
Review profile: CHILL